Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add DPT232 #5

Merged
merged 12 commits into from
Dec 31, 2024
Merged

Add DPT232 #5

merged 12 commits into from
Dec 31, 2024

Conversation

greiginsydney
Copy link
Contributor

Hi Michael,

This PR adds encoding and decoding of DPT 232.

I've never written unit tests before, but I think what I've done is valid:

pi@captureKNX-Tijl:~/tests/knxdclient $ python3 -m unittest
..Send request
Send request
.Send request
Send request
..Error while waiting for next packet from KNXd: TimeoutError(). Exiting from receive loop.
........................
----------------------------------------------------------------------
Ran 29 tests in 2.922s

OK
pi@captureKNX-Tijl:~/tests/knxdclient $

Let me know if anything's not quite right and I'll amend as required.

If you're OK with it I'll add more DPTs as users of my captureKNX bring them to my attention??

Thanks.

- Greig.

mhthies
mhthies requested changes Nov 28, 2024
View reviewed changes
Copy link
Owner

@mhthies mhthies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Greig,

thanks for your contribution! Of course, I'm happy to add more KNX DPTs! Until now I've been lazy and only added the few DPTs needed for the KNX devices at my parents' home.

However, please take a look at my comments about the Python types returned/accepted by the decode/encode functions. I'd like to keep them as consistent and semantically fitting as possible.

knxdclient/encoding.py Outdated Show resolved Hide resolved
knxdclient/encoding.py Outdated Show resolved Hide resolved
test/test_conversion.py Outdated Show resolved Hide resolved
test/test_conversion.py Outdated Show resolved Hide resolved
@greiginsydney
Copy link
Contributor Author

Thanks Michael. I'll get onto these. (Sorry for the slow response - I didn't receive any notification from GitHub that you'd responded).

@greiginsydney greiginsydney mentioned this pull request Dec 9, 2024
Closed
mhthies
mhthies approved these changes Dec 16, 2024
View reviewed changes
Copy link
Owner

@mhthies mhthies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! :)

Thanks for trying out and getting all the types to work. I have only one small suggestion for the test:

test/test_conversion.py Outdated Show resolved Hide resolved
@greiginsydney
Copy link
Contributor Author

With your consent I was going to cherry-pick a few more easy DPTs to add (into new PRs):

  • Scene Info 26 is very similar to 18 ("SCENE_INFO"?)
  • Energy 29 is the same construct as 2 x 13's, so presumably "INT64"
  • Scene config 238 is similar to 26 ("SCENE_CONTROL")
  • HVAC 212 & 213 are just combo's of 16 bit signed int's
  • HVAC 23 is just a 2-bit version of ENUM DPT 20

Maybe if I can get a few of those completed over the holidays they'd justify an updated release?

(If I get REALLY keen i might try some of the more complicated ones too, although some will require a bit of back-and-forth between us to determine the right config strategy. e.g. DPT's 21 & 22, and DPT 256, start-stop time, which is just 2 x 19's back-to-back.)

@codecov Codecov
Copy link

codecov bot commented Dec 31, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.36%. Comparing base (e9a4898) to head (12588a2).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
knxdclient/encoding.py 85.71% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master       #5      +/-   ##
==========================================
- Coverage   93.49%   93.36%   -0.13%     
==========================================
  Files           4        4              
  Lines         415      422       +7     
==========================================
+ Hits          388      394       +6     
- Misses         27       28       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mhthies mhthies merged commit b6fe17e into mhthies:master Dec 31, 2024
5 of 7 checks passed
@greiginsydney greiginsydney mentioned this pull request Dec 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhthies mhthies mhthies approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@greiginsydney @mhthies